-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
imp: MethodRoute stringValues behave as getValues #9552
base: 4.0.x
Are you sure you want to change the base?
Conversation
Kudos, SonarCloud Quality Gate passed! |
} | ||
|
||
@NonNull | ||
private String[] firstNotEmptyStringValuesInHierarchy(@NonNull String annotation, @NonNull String member) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous code is more correct, all the values are aggregated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that with:
@Controller("/example")
@RolesAllowed("isAuthenticated()")
static class ExampleController {
@Get("/admin")
@RolesAllowed({"ROLE_ADMIN", "ROLE_X"})
void admin(HttpRequest<?> request) {
}
old stringValues
returns "ROLE_ADMIN", "ROLE_X", "isAuthenticated()"
while getValues returns "ROLE_ADMIN", "ROLE_X"
. The latter looks correct to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be some behaviour inconsistency here, but this change has the potential to break a lot of things. I don't think we should be merging this 2 days before GA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok not merging it. Do you want me to target 5.0.x as the PR base?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdelamo You might need to modify the code that reads the values to only take method annotation metadata
It is too late to make a change this big IMO |
No description provided.